feat(http)!: regenerate the session identifier on authentication - #2284
feat(http)!: regenerate the session identifier on authentication#2284osbre wants to merge 4 commits into
Conversation
Benchmark ResultsComparison of Open to see the benchmark results
Generated by phpbench against commit ef862da |
brendt
left a comment
brendt
left a comment
There was a problem hiding this comment.
Why not add the regenerate logic in the manager class?
|
Since
It doesn't seem to be storage-specific, so all three managers would end up repeating the same code. It would also mean injecting the |
|
I agree with @osbre regarding the responsibility of I cleaned the API a bit by renaming methods to better names, and removing One thing I don't like but don't have a solution for is If you have any idea, I'll take it; otherwise we can just keep it as |
I understand, but then I still think it can be better designed. I really don't like replacing A couple of suggestions:
I think the third option is the best, but am open for suggestions |
|
Pushed option 3: the authenticator now takes both, with regeneration as a side effect. Let me know if this matches your vision. OWASP also recommends regenerating on other privilege changes, like password changes, 2FA, impersonation, and role escalation, so users may need to call |
| ```php | ||
| $this->session->clear(); | ||
| $this->sessionRegenerator->regenerate(); | ||
| $this->sessionManager->save($this->session); |
There was a problem hiding this comment.
Sorry for once again circling back to this, but I really find it annoying having two classes that interact with sessions. Yes I understand that it's annoying for each manager implementation to re-implement the regenerate method, but from a user's perspective it's a lot more clean if everything session-related goes through the manager.
Here's another idea: what if we keep the regenerator class, but inject it into each manager; then the SessionManager::regenerate method simply uses the underlying regenerator. I understand that this is a little more work for implementations, but it will be so much cleaner for end-users, which is much more important
3 participants
The session identifier used to survive authentication, so an identifier an attacker planted before login stayed valid afterwards and granted them the authenticated session.
SessionRegeneratorassigns a new identifier to the current session and destroys the session it replaces.SessionAuthenticatorcalls it when authenticating and deauthenticating.This is what OWASP ASVS 5.0 requires under 7.2.4 at level 1, including the termination of the previous token, and what both Laravel and Symfony do by default.
Breaking changes
SessionIdResolvergains aregenerate()method, which custom resolvers must implement.SessionAuthenticatorno longer takes aSessionManager, and takes aSessionRegeneratorinstead.